Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resolve deprecation warnings for sass #2118

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Conversation

johnnyblasta
Copy link
Collaborator

Fixes #2117

@steff-o
Copy link
Contributor

steff-o commented Jan 17, 2025

There are still deprecation warnings for glide. As it it beyond our control I guess we'll have to accept it. But is it really necessary to rebuild the stylesheets for glide? The npm package contains pre-built stylesheets, why are we not using them?

When I test to import the pre-built css-files I get the same output in style.css except that some classes have color #fff instead of white which both are white. Also I find no place where the imported scss aliases (glideCore and glideTheme) are used in our scss files. So in my opinion we could change the use statement in scss/externs/glidejs/import.scss to import the pre built css-files instead. But I'm no expert in stylesheets, so even I wouldn't trust me.

There is an issue logged for this in the glide repo: glidejs/glide#704

@steff-o
Copy link
Contributor

steff-o commented Jan 20, 2025

The glide stuff aside, this PR does not produce the same CSS as before. Basically all definitions have lost a top level selector, either .o-ui for ui-elements or .o-map for everything else. Here are two examples. but it looks the same for all definitions. Not sure if it affects anything as the selection is only widened. Maybe if map is embedded in a document that has more content than just the app wrapper. Also possibly the print control seems to search for exact style definitions const h4Rule = getCssRule('.o-ui h4, .o-ui .h4'); to resize them.

Before:

.o-ui .rounded {
  border-radius: 0.25rem;
  overflow: hidden;
}

After:

.rounded {
  border-radius: 0.25rem;
  overflow: hidden;
}

Before:

.o-map svg,
.o-map object {
  width: 100%;
}

After:

svg,
object {
  width: 100%;
}

@johnnyblasta
Copy link
Collaborator Author

Seems to be a effect of switching from @import to @use that the scope is lost. I'll try to get it back some how.

@johnnyblasta
Copy link
Collaborator Author

Do I have to edit every scss file and add the scope to all that had it before or are I'm missing something?

@steff-o
Copy link
Contributor

steff-o commented Jan 20, 2025

I surely am no export in CSS nor SASS, but from the looks of it, it is only the SASS @import extension that is deprecated, which means that the CSS @import statement is still valid inside a rule. My guess is that you start off by having a @use at the top and leave the @import inside o-map, possibly renaming all @use namespaces.

See example at https://sass-lang.com/documentation/at-rules/use/, look at the 'Loading Members' example.

Have you tried the SASS migrator?

@johnnyblasta
Copy link
Collaborator Author

johnnyblasta commented Jan 20, 2025

Now it produces correct stylesheet, had to use correct syntax for the nested imports.
The glide warnings remains though.

Copy link
Contributor

@steff-o steff-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

It produces a byte exact result (except for a comment). Probably could have fixed the glide stuff as well, but I don't know if there is a special reason for including the scss instead of the pre built css, so we'll leave it for now. Better merge before the conflicts pile up.

@steff-o steff-o added the PR approved To highlight approved PR:s as it is not shown in list which are approved. label Jan 21, 2025
@johnnyblasta
Copy link
Collaborator Author

Had to move import order to avoid a warning to last, so comment moved.

@johnnyblasta johnnyblasta merged commit ce8377f into master Jan 21, 2025
4 checks passed
@johnnyblasta johnnyblasta deleted the remove-soon-deprecated branch January 21, 2025 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR approved To highlight approved PR:s as it is not shown in list which are approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve deprecation warnings for stylesheets
2 participants